Fix for #100 #107
Fix for #100 #107
Conversation
The problem was that Vicuna accepted instructions without checking if the corresponding scalar source registers were valid. Signed-off-by: Moritz Imfeld <[email protected]>
|
I will check what caused the CI error and push another commit once I fixed it. |
There was a problem hiding this comment.
Hi @moimfeld,
I finally got around to taking a look at your PR. Your implementation looks good to me, although I am wondering if you could move some of this to the decoder module which already uses similar nested case statements to differentiate between the different types of vector instructions. That might help to reduce code duplication.
rtl/vproc_core.sv
Outdated
| source_xreg_valid = xif_issue_if.issue_req.rs_valid[0] & xif_issue_if.issue_req.rs_valid[1]; | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The decoder module uses similar nested case statements, maybe this could partially be moved to the decoder module. You could, for instance, add two output signals named needs_rs1 and needs_rs2 to the decoder and then assign source_xreg_valid in the core module as follows:
assign source_xreg_valid = (~needs_rs1 | xif_issue_if.issue_req.rs_valid[0]) & (~needs_rs2 | xif_issue_if.issue_req.rs_valid[1]);
Regarding the CI error, this does not seem to be related to your changes. The testcase that fails appears to be deadlocked due to an incorrect assignment in CV32E40X that I reported here: openhwgroup/cv32e40x#610, though I did not yet find the time to dig deeper into this and understand the exact sequence of instructions that triggers the issue. |
To avoid code duplication, the case statement in the core module has been removed. Now the case statement in the decoder is reused to decide whether a scalar source register is used for a particular instruction. To pass the signals from the decoder to the core module, I added a signal called xreg to the op_regs typedef. The vreg signal in the op_regs typedef could not be reused because it is also 0 if rs2 is not used at all or if rs2 is an immediate. Signed-off-by: Moritz Imfeld <[email protected]>
|
Hi @michael-platzer I wrote my changes into the commit message, but here is a short list of the changes:
Sidenote: I force-pushed to my fork because I accidentally put an |
|
LGTM; I squashed your commits before merging. |
To solve #100 I added a signal to the
vproc_coremodule calledsource_xreg_valid. The signal is only valid when all scalar source operands for an instruction are valid.source_xreg_validis then used to gateinstr_valid(which goes to the decoder) andxif_issue_if.issue_ready.I made the decision to implement the scalar source registers validity check in the
vproc_coremodule and not in thevproc_decodermodule because it is more related to a the x-interface than to the decoding of instructions.